-
Notifications
You must be signed in to change notification settings - Fork 62
Add server.js and backend config files for authentication #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
❌ Deploy Preview for github-spy failed.
|
WalkthroughAdds MongoDB connection and multiple Passport/local authentication implementations, introduces session-based auth routes (signup, login, logout, profile), adds a standalone JWT auth service, scaffolds server entrypoints, and installs client-side Vite + React app files and linting/configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SessionAPI as Backend (/api/auth)
participant Passport as Passport LocalStrategy
participant DB as MongoDB
Client->>SessionAPI: POST /signup { username, password }
activate SessionAPI
SessionAPI->>DB: create User (hash password)
DB-->>SessionAPI: user saved
SessionAPI-->>Client: 201 { message }
Client->>SessionAPI: POST /login { email/username, password }
SessionAPI->>Passport: authenticate('local', credentials)
activate Passport
Passport->>DB: find user by email/username
DB-->>Passport: user record
Passport->>Passport: bcrypt.compare(password, hash)
alt success
Passport-->>SessionAPI: user
SessionAPI->>SessionAPI: req.logIn(user) -> create session cookie
SessionAPI-->>Client: 200 { message, user }
else fail
Passport-->>SessionAPI: info (invalid)
SessionAPI-->>Client: 400 { message }
end
deactivate Passport
deactivate SessionAPI
Client->>SessionAPI: GET /profile (with session cookie)
SessionAPI->>SessionAPI: req.isAuthenticated()
alt authenticated
SessionAPI-->>Client: 200 { user }
else not authenticated
SessionAPI-->>Client: 401 { user: null }
end
sequenceDiagram
autonumber
actor Client
participant JWTService as github-tracker-auth
participant Bcrypt as bcryptjs
participant JWT as jsonwebtoken
Client->>JWTService: POST /register { username, password }
JWTService->>Bcrypt: hash(password)
Bcrypt-->>JWTService: hashedPassword
JWTService-->>Client: 200 { message }
Client->>JWTService: POST /login { username, password }
JWTService->>Bcrypt: compare(password, storedHash)
alt valid
JWTService->>JWT: sign({ username }, secret, 1h)
JWT-->>JWTService: token
JWTService-->>Client: 200 { token }
else invalid
JWTService-->>Client: 400 { message }
end
Client->>JWTService: GET /profile (Authorization: Bearer token)
JWTService->>JWT: verify(token)
alt ok
JWTService-->>Client: 200 { user }
else invalid
JWTService-->>Client: 401/403 { message }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thank you @gaurivvv for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/config/passportConfig.js (1)
40-44: Exclude password on deserializationPrevents leaking hashed passwords via req.user.
- const user = await User.findById(id); + const user = await User.findById(id).select('-password');
🧹 Nitpick comments (8)
server.js (1)
1-1: Clarify the entrypoint; avoid duplicate serversThere’s already a backend/server.js in the repo; this top-level server.js only logs and may confuse deploy/start scripts. Either remove it or delegate to the real server file.
Apply if you intend this to bootstrap the backend:
- console.log("Node is working!"); + // Delegate to the real backend server + require("./backend/server");backend/config/db.js (1)
10-13: Don’t exit the process inside a library helperprocess.exit(1) here will kill tests and dev watchers. Let the caller decide how to handle failures.
- } catch (err) { - console.error(err.message); - process.exit(1); - } + } catch (err) { + console.error('MongoDB connection error:', err); + throw err; + }github-tracker-auth/package.json (1)
12-18: Trim unused deps or wire them upThis package doesn’t use passport or express-session. Remove them or implement session-based flows here.
github-tracker-auth/index.js (4)
23-24: Return 201 for successful registrationCreated is the more appropriate status.
- res.json({ message: "User registered successfully 🚀" }); + res.status(201).json({ message: "User registered successfully 🚀" });
41-48: Harden Authorization header parsingBe strict about the Bearer scheme to avoid accidental parsing of malformed headers.
app.get("/profile", (req, res) => { const authHeader = req.headers["authorization"]; if (!authHeader) return res.status(401).json({ message: "No token provided" }); - const token = authHeader.split(" ")[1]; + if (!/^Bearer\s+/i.test(authHeader)) { + return res.status(401).json({ message: "Invalid auth scheme" }); + } + const token = authHeader.split(" ")[1]; jwt.verify(token, process.env.JWT_SECRET, (err, decoded) => { if (err) return res.status(403).json({ message: "Invalid token" });
54-54: Avoid fixed port; export app for testsPrevents port collisions and makes the app importable.
-app.listen(3000, () => console.log("Server is running 🚀")); +const PORT = process.env.PORT || 3000; +if (require.main === module) { + app.listen(PORT, () => console.log(`Auth server listening on ${PORT} 🚀`)); +} +module.exports = app;
8-8: Confirm this in-memory store is dev-onlyusers = [] is ephemeral; ensure the main app doesn’t depend on this service in production.
backend/routes/auth.js (1)
18-29: Add brute-force protection on loginAdd basic rate limiting on /login (and optionally /signup) to mitigate credential stuffing.
Example (outside this file’s hunk):
// near router creation const rateLimit = require('express-rate-limit'); const loginLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 20, standardHeaders: true, legacyHeaders: false }); router.post('/login', loginLimiter, /* existing handler */);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/config/db.js(1 hunks)backend/config/passportConfig.js(2 hunks)backend/routes/auth.js(1 hunks)github-tracker-auth/index.js(1 hunks)github-tracker-auth/package.json(1 hunks)server.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
server.js (1)
backend/server.js (2)
console(34-36)console(32-37)
github-tracker-auth/package.json (1)
spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
backend/config/db.js (1)
backend/server.js (2)
console(32-37)err(37-39)
github-tracker-auth/index.js (3)
backend/routes/auth.js (3)
express(1-1)req(8-8)user(10-10)backend/config/passportConfig.js (3)
user(11-11)user(42-42)isMatch(16-16)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
backend/routes/auth.js (2)
backend/server.js (2)
express(1-1)passport(4-4)backend/config/passportConfig.js (4)
passport(1-1)User(3-3)user(11-11)user(42-42)
backend/config/passportConfig.js (2)
backend/routes/auth.js (3)
passport(3-3)user(10-10)User(4-4)backend/server.js (1)
passport(4-4)
🔇 Additional comments (1)
backend/config/passportConfig.js (1)
8-9: LGTM: switch to email as usernameFieldUsing email for LocalStrategy aligns with the routes and tests.
| await mongoose.connect('mongodb://127.0.0.1:27017/githubTracker', { | ||
| useNewUrlParser: true, | ||
| useUnifiedTopology: true, | ||
| }); | ||
| console.log('MongoDB connected'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use env-configured Mongo URI; drop legacy options
Hard-coding the URI hinders env parity and the listed options are no longer needed on modern Mongoose. Prefer an env var and defaults.
- await mongoose.connect('mongodb://127.0.0.1:27017/githubTracker', {
- useNewUrlParser: true,
- useUnifiedTopology: true,
- });
- console.log('MongoDB connected');
+ const uri = process.env.MONGODB_URI || 'mongodb://127.0.0.1:27017/githubTracker';
+ await mongoose.connect(uri);
+ console.log('MongoDB connected');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await mongoose.connect('mongodb://127.0.0.1:27017/githubTracker', { | |
| useNewUrlParser: true, | |
| useUnifiedTopology: true, | |
| }); | |
| console.log('MongoDB connected'); | |
| const uri = process.env.MONGODB_URI || 'mongodb://127.0.0.1:27017/githubTracker'; | |
| await mongoose.connect(uri); | |
| console.log('MongoDB connected'); |
🤖 Prompt for AI Agents
In backend/config/db.js around lines 5 to 9, replace the hard-coded MongoDB
connection string with an env-configured URI (e.g., process.env.MONGODB_URI ||
process.env.MONGO_URI || 'mongodb://127.0.0.1:27017/githubTracker') and remove
the legacy mongoose options useNewUrlParser and useUnifiedTopology (they're not
needed on modern Mongoose); call mongoose.connect with the URI only, and
preserve/emit a clear success or error log (wrap connect in a try/catch or
handle the returned promise to log failures).
| // Local strategy for login using email | ||
| passport.use( | ||
| new LocalStrategy( | ||
| { usernameField: "email" }, | ||
| { usernameField: "email" }, // use email instead of username | ||
| async (email, password, done) => { | ||
| try { | ||
| const user = await User.findOne( {email} ); | ||
| const user = await User.findOne({ email }); | ||
| if (!user) { | ||
| return done(null, false, { message: 'Email is invalid '}); | ||
| return done(null, false, { message: 'Email is invalid' }); | ||
| } | ||
|
|
||
| const isMatch = await user.comparePassword(password); | ||
| if (!isMatch) { | ||
| return done(null, false, { message: 'Invalid password' }); | ||
| } | ||
|
|
||
| // Return user object | ||
| return done(null, { | ||
| id : user._id.toString(), | ||
| id: user._id.toString(), | ||
| username: user.username, | ||
| email: user.email | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid credential enumeration in error messages
Different messages for unknown email vs bad password allow user enumeration. Return a single generic message.
- if (!user) {
- return done(null, false, { message: 'Email is invalid' });
- }
+ if (!user) {
+ return done(null, false, { message: 'Invalid email or password' });
+ }
@@
- if (!isMatch) {
- return done(null, false, { message: 'Invalid password' });
- }
+ if (!isMatch) {
+ return done(null, false, { message: 'Invalid email or password' });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Local strategy for login using email | |
| passport.use( | |
| new LocalStrategy( | |
| { usernameField: "email" }, | |
| { usernameField: "email" }, // use email instead of username | |
| async (email, password, done) => { | |
| try { | |
| const user = await User.findOne( {email} ); | |
| const user = await User.findOne({ email }); | |
| if (!user) { | |
| return done(null, false, { message: 'Email is invalid '}); | |
| return done(null, false, { message: 'Email is invalid' }); | |
| } | |
| const isMatch = await user.comparePassword(password); | |
| if (!isMatch) { | |
| return done(null, false, { message: 'Invalid password' }); | |
| } | |
| // Return user object | |
| return done(null, { | |
| id : user._id.toString(), | |
| id: user._id.toString(), | |
| username: user.username, | |
| email: user.email | |
| }); | |
| // Local strategy for login using email | |
| passport.use( | |
| new LocalStrategy( | |
| { usernameField: "email" }, // use email instead of username | |
| async (email, password, done) => { | |
| try { | |
| const user = await User.findOne({ email }); | |
| if (!user) { | |
| return done(null, false, { message: 'Invalid email or password' }); | |
| } | |
| const isMatch = await user.comparePassword(password); | |
| if (!isMatch) { | |
| return done(null, false, { message: 'Invalid email or password' }); | |
| } | |
| // Return user object | |
| return done(null, { | |
| id: user._id.toString(), | |
| username: user.username, | |
| email: user.email | |
| }); |
🤖 Prompt for AI Agents
In backend/config/passportConfig.js around lines 5 to 26, the LocalStrategy
currently returns different messages for "Email is invalid" and "Invalid
password", which enables credential enumeration; change both failure responses
to a single generic message (e.g., "Invalid email or password") and ensure
timing does not reveal existence (if user not found perform a dummy password
compare or a consistent async delay) so both unknown-email and wrong-password
cases behave identically.
| router.post('/signup', async (req, res) => { | ||
| const { username, email, password } = req.body; | ||
| try { | ||
| const user = new User({ username, email, password }); | ||
| await user.save(); | ||
| res.status(201).json({ message: 'User created successfully' }); | ||
| } catch (err) { | ||
| res.status(400).json({ error: err.message }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Align responses with tests and standardize JSON shape
Tests in spec/auth.routes.spec.cjs assert:
- 201 + "User created successfully" on signup.
- 400 + "User already exists" on duplicate.
- 200 + "Login successful" on login.
- 401 on wrong password.
- 200 + "Logged out successfully" on logout.
Ensure all responses use { message: string } for messages and selected/safe user payloads only.
Run to sanity-check strings/statuses without executing tests:
Also applies to: 19-29, 32-36, 39-41
🏁 Script executed:
#!/bin/bash
rg -nC1 -e "User created successfully|User already exists|Login successful|Logged out successfully" backend/routes/auth.js
rg -nC2 -e "status\\(401\\)|status\\(400\\)" backend/routes/auth.js
rg -nC3 -e "expect\\(res.body.message\\)" spec/auth.routes.spec.cjsLength of output: 2224
Align auth routes with spec: status codes & JSON shape
- Signup catch: use
res.status(400).json({ message: err.message })(noterror), and specifically detect duplicate‐key errors to return{ message: 'User already exists' }. - Login failure: return
res.status(401).json({ message: info.message })for invalid credentials (not 400). - Login success: respond with
res.status(200).json({ message: 'Login successful', user: /* sanitized user fields */ })instead of rawres.json({ user }). - Logout already returns the correct 200 +
{ message }.
🤖 Prompt for AI Agents
In backend/routes/auth.js around lines 7-16, the signup error handling and login
responses must match the API spec: in the signup catch block replace
res.status(400).json({ error: err.message }) with res.status(400).json({
message: err.message }) and detect duplicate-key errors (MongoDB code 11000 or
err.codeName === 'DuplicateKey') to instead return res.status(400).json({
message: 'User already exists' }); for login failures return
res.status(401).json({ message: info.message }) (not 400), and on login success
respond with res.status(200).json({ message: 'Login successful', user: /*
sanitized user fields */ }) where you omit sensitive fields (e.g., password,
tokens) and only include safe fields like id, username, email; leave logout
as-is.
💡 Verification agent
🧩 Analysis chain
Signup: handle duplicate emails, validate inputs, and standardize error shape
- No input validation; add required-field checks.
- Duplicate email relies on DB error and returns
{ error: ... }, while tests expect{ message: 'User already exists' }with 400. - Avoid leaking raw DB error strings.
Apply:
router.post('/signup', async (req, res) => {
- const { username, email, password } = req.body;
+ const { username, email, password } = req.body;
+ if (!username || !email || !password) {
+ return res.status(400).json({ message: 'username, email and password are required' });
+ }
try {
const user = new User({ username, email, password });
await user.save();
- res.status(201).json({ message: 'User created successfully' });
+ return res.status(201).json({ message: 'User created successfully' });
} catch (err) {
- res.status(400).json({ error: err.message });
+ if (err && err.code === 11000) {
+ // duplicate key (e.g., unique email)
+ return res.status(400).json({ message: 'User already exists' });
+ }
+ return res.status(400).json({ message: 'Invalid signup data' });
}
});Confirm the User model enforces a unique index on email and hashes passwords (e.g., pre-save with bcrypt). If not, I can provide a patch.
🏁 Script executed:
#!/bin/bash
# Locate the User model file in backend/models
user_model_file=$(rg -l "mongoose\\.model\\(['\"]User['\"]" -g "backend/models/*.js")
echo "User model file: $user_model_file"
# Check if email field has a unique constraint
echo "--- Checking unique constraint on email ---"
rg -n "email\\s*:\\s*\\{[^}]*unique" "$user_model_file" || echo "No unique constraint found on email"
# Check for bcrypt import or usage
echo "--- Checking for bcrypt import ---"
rg -n "require\\(['\"]bcrypt['\"]\\)" "$user_model_file" || echo "No bcrypt import found"
echo "--- Checking for bcrypt usage ---"
rg -n "bcrypt\\." "$user_model_file" || echo "No bcrypt usage found"
# Check for pre-save hook for password hashing
echo "--- Checking for pre\\('save' hook ---"
rg -n "pre\\('save'" "$user_model_file" || echo "No pre-save hook found"Length of output: 1129
🏁 Script executed:
#!/bin/bash
file=backend/models/User.js
echo "--- Searching for schema.index usage ---"
rg -n "UserSchema\\.index" "$file" || echo "No schema.index usage"
echo "--- Searching for inline unique on email ---"
rg -n "'email'\\s*:\\s*\\{[^}]*unique" "$file" || echo "No inline unique on email"
echo "--- Searching for ES6 import of bcrypt ---"
rg -n "import .*bcrypt" "$file" || echo "No ES6 import bcrypt"
echo "--- Searching for require('bcrypt') ---"
rg -n "require\\(['\"]bcrypt['\"]\\)" "$file" || echo "No require bcrypt"Length of output: 851
Enforce unique email index in User model
- Passwords are hashed via the
pre('save')hook in backend/models/User.js. - The
emailfield lacks a unique constraint—addunique: trueon the field orUserSchema.index({ email: 1 }, { unique: true })to prevent duplicates.
🤖 Prompt for AI Agents
In backend/routes/auth.js lines 7-16: the User model currently allows duplicate
emails; open backend/models/User.js and add a uniqueness constraint for email
(either set the email field option unique: true or call UserSchema.index({
email: 1 }, { unique: true })), then ensure indexes are built (e.g., run
mongoose models' ensureIndexes/createIndexes on startup or after deployment);
finally, keep existing signup error handling but optionally map Mongo duplicate
key errors (E11000) to a 409 conflict for clearer client feedback.
| router.post('/login', (req, res, next) => { | ||
| passport.authenticate('local', (err, user, info) => { | ||
| if (err) return next(err); | ||
| if (!user) return res.status(400).json({ message: info.message }); | ||
|
|
||
| const newUser = new User( {username, email, password} ); | ||
| await newUser.save(); | ||
| res.status(201).json( {message: 'User created successfully'} ); | ||
| } catch (err) { | ||
| res.status(500).json({ message: 'Error creating user', error: err.message }); | ||
| } | ||
| req.login(user, (err) => { | ||
| if (err) return next(err); | ||
| res.json({ message: 'Logged in successfully', user }); | ||
| }); | ||
| })(req, res, next); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Login: return 401 on bad creds, align success message with tests, sanitize user, and prevent session fixation
- Failure uses 400; use 401.
- Tests expect message "Login successful" (not "Logged in successfully").
- Returning the full user risks leaking hashed password/PII.
- Regenerate session on login.
Apply:
router.post('/login', (req, res, next) => {
passport.authenticate('local', (err, user, info) => {
if (err) return next(err);
- if (!user) return res.status(400).json({ message: info.message });
+ if (!user) {
+ return res.status(401).json({ message: (info && info.message) || 'Invalid email or password' });
+ }
- req.login(user, (err) => {
- if (err) return next(err);
- res.json({ message: 'Logged in successfully', user });
- });
+ const finish = () => {
+ req.login(user, (err) => {
+ if (err) return next(err);
+ const { _id, username, email } = user;
+ return res.json({ message: 'Login successful', user: { id: _id, username, email } });
+ });
+ };
+ if (req.session && typeof req.session.regenerate === 'function') {
+ return req.session.regenerate((regenErr) => (regenErr ? next(regenErr) : finish()));
+ }
+ return finish();
})(req, res, next);
});Ensure passport/session middleware is configured before these routes (express-session, passport.initialize(), passport.session()).
🏁 Script executed:
#!/bin/bash
# Check tests for expected login messages and status codes
echo "=== Login message references ==="
rg -n "Login successful" || echo 'No occurrences of "Login successful" found'
rg -n "Logged in successfully" || echo 'No occurrences of "Logged in successfully" found'
echo
echo "=== Status code expectations in tests ==="
rg -n "\.expect(400)" || echo 'No .expect(400) in tests'
rg -n "\.expect(401)" || echo 'No .expect(401) in tests'
echo
echo "=== User schema definitions ==="
rg -l "new Schema" -g "*user*.js" || echo 'No user schema files found'
rg -n "new Schema" -g "*user*.js" -C3
echo "Checking user schema fields:"
rg -nE "username|email|password" -g "*user*.js" -C1 || echo 'Unable to locate username/email/password fields in user schema'
echo
echo "=== Passport & session middleware ==="
rg -n "passport.initialize" -C2 || echo 'passport.initialize not configured'
rg -n "express-session" -C2 || echo 'express-session not configured'
rg -n "passport.session" -C2 || echo 'passport.session not configured'
echo
echo "=== Auth routes mounting ==="
rg -n "routes/auth" -C2
rg -n "use.*auth" -g "*app*.js" -C2 || echo 'Auth routes not found in app setup'Length of output: 5862
🏁 Script executed:
# Display the beginning of the test suite to verify status code assertions for login failures
sed -n '1,200p' spec/auth.routes.spec.cjsLength of output: 3393
backend/routes/auth.js – Login: use 401 on invalid credentials, match test message, sanitize user, regenerate session
- Change failure status to 401 (tests expect 401 on bad creds) and provide a safe fallback on
info.message. - Change success message to “Login successful” and return only
{ id, username, email }(avoid leaking hashes/PII). - Regenerate the session before calling
req.login()to prevent session fixation.
Apply:
router.post('/login', (req, res, next) => {
passport.authenticate('local', (err, user, info) => {
if (err) return next(err);
- if (!user) return res.status(400).json({ message: info.message });
+ if (!user) {
+ return res.status(401).json({ message: (info && info.message) || 'Invalid email or password' });
+ }
- req.login(user, (err) => {
- if (err) return next(err);
- res.json({ message: 'Logged in successfully', user });
- });
+ const finish = () => {
+ req.login(user, (err) => {
+ if (err) return next(err);
+ const { _id, username, email } = user;
+ return res.json({ message: 'Login successful', user: { id: _id, username, email } });
+ });
+ };
+ if (req.session && typeof req.session.regenerate === 'function') {
+ return req.session.regenerate((regenErr) => (regenErr ? next(regenErr) : finish()));
+ }
+ return finish();
})(req, res, next);
});Session and Passport middleware are already configured before these routes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.post('/login', (req, res, next) => { | |
| passport.authenticate('local', (err, user, info) => { | |
| if (err) return next(err); | |
| if (!user) return res.status(400).json({ message: info.message }); | |
| const newUser = new User( {username, email, password} ); | |
| await newUser.save(); | |
| res.status(201).json( {message: 'User created successfully'} ); | |
| } catch (err) { | |
| res.status(500).json({ message: 'Error creating user', error: err.message }); | |
| } | |
| req.login(user, (err) => { | |
| if (err) return next(err); | |
| res.json({ message: 'Logged in successfully', user }); | |
| }); | |
| })(req, res, next); | |
| }); | |
| router.post('/login', (req, res, next) => { | |
| passport.authenticate('local', (err, user, info) => { | |
| if (err) return next(err); | |
| if (!user) { | |
| return res.status(401).json({ message: (info && info.message) || 'Invalid email or password' }); | |
| } | |
| const finish = () => { | |
| req.login(user, (err) => { | |
| if (err) return next(err); | |
| const { _id, username, email } = user; | |
| return res.json({ message: 'Login successful', user: { id: _id, username, email } }); | |
| }); | |
| }; | |
| if (req.session && typeof req.session.regenerate === 'function') { | |
| return req.session.regenerate((regenErr) => (regenErr ? next(regenErr) : finish())); | |
| } | |
| return finish(); | |
| })(req, res, next); | |
| }); |
🤖 Prompt for AI Agents
In backend/routes/auth.js around lines 19 to 29, the login handler must return
401 on invalid credentials and use a safe fallback for the passport
info.message; regenerate the session before calling req.login() to prevent
session fixation; on success respond with the message "Login successful" and
only return a sanitized user object containing { id, username, email } (no
password or extra PII); ensure you handle errors from req.session.regenerate and
from req.login and propagate them to next(err) or send appropriate responses.
| router.get('/logout', (req, res) => { | ||
| req.logout(() => { | ||
| res.json({ message: 'Logged out successfully' }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Logout: destroy the session and clear cookie; keep GET for now but harden flow
- Current GET route doesn’t destroy the session/cookie.
- Keeping GET to match tests is OK, but add session destruction. Consider also exposing POST /logout to reduce CSRF risk.
Apply:
-router.get('/logout', (req, res) => {
- req.logout(() => {
- res.json({ message: 'Logged out successfully' });
- });
-});
+router.get('/logout', (req, res, next) => {
+ req.logout((err) => {
+ if (err) return next(err);
+ if (req.session) {
+ return req.session.destroy(() => {
+ res.clearCookie('connect.sid');
+ return res.json({ message: 'Logged out successfully' });
+ });
+ }
+ return res.json({ message: 'Logged out successfully' });
+ });
+});Optionally add (outside this hunk) a POST variant mirroring the above to discourage CSRFable state changes via GET.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get('/logout', (req, res) => { | |
| req.logout(() => { | |
| res.json({ message: 'Logged out successfully' }); | |
| }); | |
| }); | |
| router.get('/logout', (req, res, next) => { | |
| req.logout((err) => { | |
| if (err) return next(err); | |
| if (req.session) { | |
| return req.session.destroy(() => { | |
| res.clearCookie('connect.sid'); | |
| return res.json({ message: 'Logged out successfully' }); | |
| }); | |
| } | |
| return res.json({ message: 'Logged out successfully' }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In backend/routes/auth.js around lines 32 to 36, the logout GET handler
currently calls req.logout but does not destroy the session or clear the session
cookie; update the handler to call req.logout (preserving the callback), then
call req.session.destroy with an error check and in its callback call
res.clearCookie using the session cookie name (e.g. 'connect.sid' or the app's
session cookie name) before sending the JSON response; handle errors by
returning a 500 or appropriate error response if session destroy fails. Also
optionally add a POST /logout variant (outside this hunk) that mirrors the same
destruction/clear-cookie flow to discourage CSRF via GET.
| router.get('/profile', (req, res) => { | ||
| if (!req.isAuthenticated()) return res.status(401).json({ message: 'Not authenticated' }); | ||
| res.json({ user: req.user }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Profile: don’t return the full user object
Return a minimal, safe shape to avoid leaking sensitive fields.
Apply:
router.get('/profile', (req, res) => {
if (!req.isAuthenticated()) return res.status(401).json({ message: 'Not authenticated' });
- res.json({ user: req.user });
+ const { _id, username, email } = req.user;
+ return res.json({ user: { id: _id, username, email } });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get('/profile', (req, res) => { | |
| if (!req.isAuthenticated()) return res.status(401).json({ message: 'Not authenticated' }); | |
| res.json({ user: req.user }); | |
| router.get('/profile', (req, res) => { | |
| if (!req.isAuthenticated()) return res.status(401).json({ message: 'Not authenticated' }); | |
| const { _id, username, email } = req.user; | |
| return res.json({ user: { id: _id, username, email } }); | |
| }); |
🤖 Prompt for AI Agents
In backend/routes/auth.js around lines 39-41, the profile route currently
returns the full req.user which can leak sensitive fields; change the response
to return a minimal safe user shape (for example: id, email, name, and any
non-sensitive role/flags) by selecting those specific properties from req.user
before sending, explicitly excluding sensitive fields such as password, tokens,
social IDs, or internal metadata; keep the existing authentication check and
status codes but ensure the returned object is a sanitized DTO.
| const express = require("express"); | ||
| const bcrypt = require("bcryptjs"); | ||
| const jwt = require("jsonwebtoken"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded JWT secret in code
Secret should come from env and not be committed.
-const express = require("express");
-const bcrypt = require("bcryptjs");
-const jwt = require("jsonwebtoken");
+const express = require("express");
+const bcrypt = require("bcryptjs");
+const jwt = require("jsonwebtoken");
+require("dotenv").config();
@@
- const token = jwt.sign({ username }, "secretKey123", { expiresIn: "1h" });
+ const token = jwt.sign({ username }, process.env.JWT_SECRET, { expiresIn: "1h" });
@@
- jwt.verify(token, "secretKey123", (err, decoded) => {
+ jwt.verify(token, process.env.JWT_SECRET, (err, decoded) => {Also applies to: 37-38, 47-48
🤖 Prompt for AI Agents
In github-tracker-auth/index.js around lines 1-4 (and also update occurrences at
37-38 and 47-48), the JWT secret is hard-coded; change the code to read the
secret from an environment variable (e.g., process.env.JWT_SECRET) instead of
embedding it, add a startup-time check that exits or throws a clear error if the
env var is missing, and update all places that reference the hard-coded value to
use that env var; optionally load from .env in development (dotenv) but do not
commit any secret values.
| app.post("/register", async (req, res) => { | ||
| const { username, password } = req.body; | ||
|
|
||
| // check if user exists | ||
| const existingUser = users.find(u => u.username === username); | ||
| if (existingUser) { | ||
| return res.status(400).json({ message: "User already exists" }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add minimal input validation for auth endpoints
Reject empty/missing username/password to avoid ambiguous states.
app.post("/register", async (req, res) => {
const { username, password } = req.body;
+ if (!username || !password) {
+ return res.status(400).json({ message: "Username and password are required" });
+ }
+
// check if user exists
const existingUser = users.find(u => u.username === username);
if (existingUser) {
return res.status(400).json({ message: "User already exists" });
}
@@
app.post("/login", async (req, res) => {
const { username, password } = req.body;
+ if (!username || !password) {
+ return res.status(400).json({ message: "Username and password are required" });
+ }
+
const user = users.find(u => u.username === username);
if (!user) return res.status(400).json({ message: "Invalid credentials" });
const isMatch = await bcrypt.compare(password, user.password);
if (!isMatch) return res.status(400).json({ message: "Invalid credentials" });Also applies to: 27-35
🤖 Prompt for AI Agents
In github-tracker-auth/index.js around lines 11-18 (and also apply same changes
to lines 27-35), the register/login handlers lack input validation for
username/password; ensure both fields are present and non-empty strings: trim
inputs, verify typeof === "string" and length > 0, and if validation fails
return res.status(400).json({ message: "Username and password are required" });
perform the same checks before any user lookup/auth logic to avoid ambiguous
states.
| "dependencies": { | ||
| "bcryptjs": "^3.0.2", | ||
| "express": "^5.1.0", | ||
| "express-session": "^1.18.2", | ||
| "passport": "^0.7.0", | ||
| "passport-local": "^1.0.0" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing jsonwebtoken dependency will crash at runtime
index.js requires jsonwebtoken but it isn’t declared.
"dependencies": {
"bcryptjs": "^3.0.2",
"express": "^5.1.0",
"express-session": "^1.18.2",
"passport": "^0.7.0",
- "passport-local": "^1.0.0"
+ "passport-local": "^1.0.0",
+ "jsonwebtoken": "^9.0.0"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dependencies": { | |
| "bcryptjs": "^3.0.2", | |
| "express": "^5.1.0", | |
| "express-session": "^1.18.2", | |
| "passport": "^0.7.0", | |
| "passport-local": "^1.0.0" | |
| } | |
| "dependencies": { | |
| "bcryptjs": "^3.0.2", | |
| "express": "^5.1.0", | |
| "express-session": "^1.18.2", | |
| "passport": "^0.7.0", | |
| "passport-local": "^1.0.0", | |
| "jsonwebtoken": "^9.0.0" | |
| } |
🤖 Prompt for AI Agents
In github-tracker-auth/package.json around lines 12 to 18, index.js requires
jsonwebtoken but it is not declared in dependencies; add a dependency entry for
"jsonwebtoken" (e.g. "jsonwebtoken": "^9.0.0") to the dependencies object in
package.json and then run npm install (or yarn) to update node_modules and the
lockfile so the package is available at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 33
🧹 Nitpick comments (15)
client/vite.config.js (1)
5-7: Dev DX: add proxy to backend for cookie-based auth (withCredentials).Proxying
/apito your backend avoids CORS pain and forwards cookies during dev.-import { defineConfig } from 'vite' +import { defineConfig, loadEnv } from 'vite' import react from '@vitejs/plugin-react' // https://vite.dev/config/ -export default defineConfig({ - plugins: [react()], -}) +export default defineConfig(({ mode }) => { + const env = loadEnv(mode, process.cwd(), ''); + return { + plugins: [react()], + server: { + proxy: { + '/api': { + target: env.VITE_BACKEND_URL || 'http://localhost:3000', + changeOrigin: true, + secure: false, + } + } + } + } +})client/.gitignore (1)
15-24: Ignore env files to avoid leaking secrets.Add typical Vite env patterns; keep
.env.exampletracked.# Editor directories and files .vscode/* !.vscode/extensions.json .idea .DS_Store *.suo *.ntvs* *.njsproj *.sln *.sw? +# +# Env files +.env +.env.local +.env.development.local +.env.test.local +.env.production.local +.env.*.local +!.env.exampleclient/package.json (1)
12-28: Toolchain compatibility confirmed.
• @vitejs/plugin-react v5.x (latest5.0.2) peer-depends on Vite^4.2.0 || ^5.0.0 || ^6.0.0 || ^7.0.0, so Vite 7 is supported.
• react-router-dom 7.8.2 peer-depends on React>=18and React-DOM>=18, so React 19/React-DOM 19 satisfy it.Consider pinning Node for consistent ESM/Vite support (optional):
"type": "module", + "engines": { + "node": ">=18.18.0" + },middleware/auth.js (1)
1-13: Unify response shape (messagevsmsg) and status codes.Elsewhere you use
message. Align for consistency.export function ensureAuthenticated(req, res, next) { if (req.isAuthenticated()) { return next(); } - res.status(401).json({ msg: 'You must be logged in to access this route' }); + res.status(401).json({ message: 'You must be logged in to access this route' }); } export function forwardAuthenticated(req, res, next) { if (!req.isAuthenticated()) { return next(); } - res.status(403).json({ msg: 'You are already logged in' }); + res.status(403).json({ message: 'You are already logged in' }); }src/Signup.jsx (1)
5-29: Duplicate Signup implementations (TSX vs JSX) risk divergence.There’s also src/pages/Signup/Signup.tsx using username/email/password and different API shape. Keep one source of truth.
Would you like me to generate a migration diff to consolidate on the TSX page and remove this JSX file?
server.js (2)
38-41: Remove deprecated Mongoose options (v6+).useNewUrlParser and useUnifiedTopology are defaults and can be dropped.
21-27: Behind a reverse proxy, set trust proxy for secure cookies.Apply:
const app = express(); +if (process.env.NODE_ENV === 'production') app.set('trust proxy', 1);src/dashboard.jsx (1)
5-5: Prop usage nit: 'user' isn’t referenced.Remove or use it to personalize the message.
src/Routes/auth.js (2)
23-26: Handle duplicate key errors from MongoDBGuard against race conditions when two signups use the same username.
Apply:
- } catch (err) { - res.status(500).json({ error: err.message }); - } + } catch (err) { + if (err?.code === 11000) { + return res.status(400).json({ message: 'Username already exists' }); + } + res.status(500).json({ message: 'Error creating user' }); + }
10-20: Add basic input validationEnforce username format and password strength (length/min classes), ideally via express-validator/Joi, before hashing/saving.
client/src/main.jsx (1)
6-10: Guard against missing #rootAvoid runtime errors if the root element is absent.
Apply:
-createRoot(document.getElementById('root')).render( +const rootEl = document.getElementById('root'); +if (!rootEl) throw new Error('#root not found'); +createRoot(rootEl).render(src/login.jsx (1)
5-19: Duplicate Login implementations exist (this and src/pages/Login/Login.tsx)Keep one source of truth to avoid drift.
client/eslint.config.js (1)
11-15: Replace deprecated React Hooks preset and include React/a11y configs
- Swap
reactHooks.configs['recommended-latest']→reactHooks.configs.recommended
(recommended-latestis deprecated as of v5.2.0; the flat-configrecommendedis now the default) (npmjs.com, app.semanticdiff.com)- Add React and a11y shareable configs to the
extendsarray:
react.configs.flat.recommendedandjsxA11y.flatConfigs.recommended(app.unpkg.com, github.com)server/server.js (1)
8-8: Consider removing redundant body-parser dependency.Express 4.16+ includes built-in JSON and URL-encoded body parsing. The
body-parserpackage is redundant and can be replaced withexpress.json()andexpress.urlencoded().-import bodyParser from 'body-parser';-app.use(bodyParser.json()); -app.use(bodyParser.urlencoded({ extended: true })); +app.use(express.json()); +app.use(express.urlencoded({ extended: true }));package.json (1)
14-14: Remove redundant body-parser dependency.The
body-parserpackage is redundant since Express 4.16+ includes built-in JSON and URL-encoded parsing middleware.- "body-parser": "^1.20.2",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/public/vite.svgis excluded by!**/*.svgclient/src/assets/react.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
auth.js(1 hunks)backend/config/passport.js(1 hunks)client/.gitignore(1 hunks)client/README.md(1 hunks)client/eslint.config.js(1 hunks)client/index.html(1 hunks)client/package.json(1 hunks)client/src/App.css(1 hunks)client/src/App.jsx(1 hunks)client/src/index.css(1 hunks)client/src/main.jsx(1 hunks)client/vite.config.js(1 hunks)config/passport.js(1 hunks)middleware/auth.js(1 hunks)models/User.js(1 hunks)package.json(1 hunks)routes/auth.js(1 hunks)server.js(1 hunks)server/server.js(1 hunks)src/App.jsx(1 hunks)src/App.tsx(1 hunks)src/Routes/Router.tsx(1 hunks)src/Routes/auth.js(1 hunks)src/Signup.jsx(1 hunks)src/dashboard.jsx(1 hunks)src/login.jsx(1 hunks)src/main.tsx(1 hunks)tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- client/index.html
- client/README.md
- client/src/App.css
🧰 Additional context used
🧬 Code graph analysis (15)
middleware/auth.js (1)
routes/auth.js (1)
req(11-11)
config/passport.js (2)
backend/config/passportConfig.js (1)
LocalStrategy(2-2)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
src/login.jsx (3)
src/Signup.jsx (3)
username(6-6)password(7-7)navigate(8-8)src/dashboard.jsx (1)
navigate(7-7)src/pages/Login/Login.tsx (3)
e(28-44)formData(14-160)e(23-26)
tailwind.config.js (1)
src/context/ThemeContext.tsx (1)
ThemeContextType(5-8)
src/Signup.jsx (1)
src/pages/Signup/Signup.tsx (3)
formData(12-160)SignUpFormData(6-10)e(20-23)
backend/config/passport.js (3)
backend/config/passportConfig.js (6)
passport(1-1)LocalStrategy(2-2)user(11-11)user(42-42)User(3-3)isMatch(16-16)backend/routes/auth.js (3)
passport(3-3)user(10-10)User(4-4)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
server/server.js (2)
config/passport.js (1)
configurePassport(6-33)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
auth.js (1)
routes/auth.js (2)
router(7-7)req(11-11)
client/vite.config.js (1)
src/vite-env.d.ts (2)
ImportMeta(8-10)ImportMetaEnv(3-6)
src/Routes/auth.js (3)
routes/auth.js (3)
req(11-11)existingUser(18-18)newUser(24-24)backend/config/passport.js (2)
user(10-10)user(32-32)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
src/Routes/Router.tsx (3)
src/pages/Contributors/Contributors.tsx (3)
Contributor(19-25)contributors(27-142)contributor(74-137)src/pages/Contact/Contact.tsx (1)
Contact(13-349)src/pages/About/About.tsx (1)
div(22-85)
src/main.tsx (3)
src/login.jsx (1)
Login(5-29)src/Signup.jsx (1)
Signup(5-29)src/dashboard.jsx (1)
Dashboard(5-34)
routes/auth.js (2)
backend/config/passport.js (2)
user(10-10)user(32-32)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
server.js (4)
backend/routes/auth.js (2)
express(1-1)passport(3-3)backend/server.js (2)
console(32-37)err(37-39)backend/config/passportConfig.js (1)
passport(1-1)spec/auth.routes.spec.cjs (1)
createTestApp(10-19)
src/App.jsx (3)
src/login.jsx (1)
Login(5-29)src/Signup.jsx (1)
Signup(5-29)src/dashboard.jsx (1)
Dashboard(5-34)
🪛 Biome (2.1.2)
client/src/App.jsx
[error] 12-12: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 15-15: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
src/App.jsx
[error] 6-6: Shouldn't redeclare 'React'. Consider to delete it or rename it.
'React' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (11)
backend/config/passport.js (1)
24-37: LGTM on session (de)serialization.
serializeUserstores id;deserializeUserfetches user with-password. Good.server.js (1)
1-7: ESM support confirmed. package.json specifies"type": "module", so your ES module imports in server.js are valid.src/Routes/Router.tsx (1)
3-5: LGTM on explicit extensionsImports resolve correctly and match paths under src/pages. No functional concerns.
src/login.jsx (1)
13-18: Confirm backend auth endpoint prefix and response schema
- No Express routes found in this repo—please verify with the backend that the login endpoint is mounted at
/api/auth/login(not/auth/login) and that responses use eithermessageormsgkeys so the front end can handle both.routes/auth.js (1)
10-15: LGTM! Clean user session management implementation.The useEffect hook correctly fetches the current user on component mount and handles both success and error cases appropriately. The user state management with
setUserprovides a clean foundation for session-aware routing.server/server.js (1)
44-50: LGTM! Proper MongoDB connection with error handling.The MongoDB connection setup correctly uses environment variables with sensible defaults and includes proper error handling for both success and failure cases.
client/src/index.css (1)
1-69: LGTM! Well-structured CSS with accessibility and theming support.The CSS provides a solid foundation with:
- System font stack and proper typography settings
- Dark/light theme support with
prefers-color-scheme- Accessible focus states for interactive elements
- Consistent color scheme and spacing
The implementation follows modern CSS best practices.
config/passport.js (1)
6-33: Ignore duplication concern:config/passport.jsandbackend/config/passportConfig.jslive in separate service contexts (imported only byserver/server.jsvsbackend/server.js) and aren’t cross-imported, so there’s no conflict.Likely an incorrect or invalid review comment.
package.json (3)
2-5: LGTM! Proper project metadata setup.The package metadata correctly identifies this as a backend server project with appropriate name, version, description, and main entry point.
24-28: LGTM! Appropriate development dependencies.The development dependencies are well-chosen for backend development with proper testing setup (Jasmine, Supertest) and development workflow (Nodemon).
12-23: Review major version upgrades for Express and Mongoose
- express: currently ^4.18.2, latest stable 5.1.0 (major)
- mongoose: currently ^7.5.0, latest stable 8.15.1 (major)
- passport (0.7.0) and bcryptjs (3.0.2) are already at latest
Confirm compatibility and test for breaking changes before bumping these major versions.
| router.get("/logout", (req, res) => { | ||
| req.logout(err => { | ||
| if(err) return res.status(500).json({ message: "Logout failed" }); | ||
| res.json({ message: "Logged out successfully" }); | ||
| }); | ||
| }); | ||
|
|
||
| // Get logged-in user | ||
| router.get("/current", (req, res) => { | ||
| if (req.isAuthenticated()) { | ||
| res.json({ user: { username: req.user.username } }); | ||
| } else { | ||
| res.status(401).json({ user: null }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Router not declared/exported; file won’t work as-is.
Add Router setup and export, and return status codes explicitly.
+import express from 'express';
+const router = express.Router();
+
router.get("/logout", (req, res) => {
req.logout(err => {
- if(err) return res.status(500).json({ message: "Logout failed" });
- res.json({ message: "Logged out successfully" });
+ if (err) return res.status(500).json({ message: "Logout failed" });
+ return res.status(200).json({ message: "Logged out successfully" });
});
});
// Get logged-in user
router.get("/current", (req, res) => {
if (req.isAuthenticated()) {
- res.json({ user: { username: req.user.username } });
+ return res.status(200).json({ user: { username: req.user.username } });
} else {
- res.status(401).json({ user: null });
+ return res.status(401).json({ user: null });
}
});
+
+export default router;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get("/logout", (req, res) => { | |
| req.logout(err => { | |
| if(err) return res.status(500).json({ message: "Logout failed" }); | |
| res.json({ message: "Logged out successfully" }); | |
| }); | |
| }); | |
| // Get logged-in user | |
| router.get("/current", (req, res) => { | |
| if (req.isAuthenticated()) { | |
| res.json({ user: { username: req.user.username } }); | |
| } else { | |
| res.status(401).json({ user: null }); | |
| } | |
| }); | |
| import express from 'express'; | |
| const router = express.Router(); | |
| router.get("/logout", (req, res) => { | |
| req.logout(err => { | |
| if (err) return res.status(500).json({ message: "Logout failed" }); | |
| return res.status(200).json({ message: "Logged out successfully" }); | |
| }); | |
| }); | |
| // Get logged-in user | |
| router.get("/current", (req, res) => { | |
| if (req.isAuthenticated()) { | |
| return res.status(200).json({ user: { username: req.user.username } }); | |
| } else { | |
| return res.status(401).json({ user: null }); | |
| } | |
| }); | |
| export default router; |
🤖 Prompt for AI Agents
In auth.js around lines 1-15, the file uses router but never declares or exports
it and some responses lack explicit status codes; fix by importing or requiring
express and creating const router = express.Router() at the top, replace plain
res.json success responses with explicit status codes (e.g., send 200 for
successful logout and current-user responses, keep 500 for logout error and 401
for unauthorized), and export the router at the bottom (module.exports = router
or export default router consistent with the project style).
| import passport from 'passport'; | ||
| import { Strategy as LocalStrategy } from 'passport-local'; | ||
| import bcrypt from 'bcryptjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Consolidate duplicate Passport configs.
Both backend/config/passport.js (ESM) and backend/config/passportConfig.js (CJS) initialize LocalStrategy. Keep one to avoid double registration and drift.
🏁 Script executed:
#!/bin/bash
# Locate all passport strategy registrations to remove duplicates.
rg -nP -C2 "\bpassport\.use\(\s*new\s+LocalStrategy"Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating passportConfig.js"
rg --files | grep passportConfig.js || echo "No passportConfig.js found"
echo
echo "Locating passport.js"
rg --files | grep 'config/passport.js' || echo "No passport.js found"
echo
echo "Searching for passport.use occurrences"
rg -n "passport\.use" || echo "No passport.use calls found"
echo
echo "Searching for new LocalStrategy"
rg -n "new LocalStrategy" || echo "No new LocalStrategy instantiations found"
echo
echo "Searching for passport-local imports"
rg -n "passport-local" || echo "No passport-local imports found"Length of output: 1143
Consolidate Passport strategy registrations. We have three separate LocalStrategy initializations in config/passport.js, backend/config/passport.js, and backend/config/passportConfig.js. Centralize into a single module and remove the duplicates to prevent multiple registrations.
🤖 Prompt for AI Agents
In backend/config/passport.js around lines 1 to 3, there are duplicate
LocalStrategy initializations across config/passport.js,
backend/config/passport.js, and backend/config/passportConfig.js; consolidate
them into a single module: pick one canonical file (e.g.,
backend/config/passport.js), move the complete LocalStrategy registration and
related serialization/deserialization logic into that file, remove the duplicate
strategy registrations and imports from the other files (or have them import and
use the single canonical module), and ensure only the canonical module calls
passport.use(...) so the LocalStrategy is registered exactly once.
| passport.use( | ||
| new LocalStrategy(async (username, password, done) => { | ||
| try { | ||
| // Find user by username | ||
| const user = await User.findOne({ username }); | ||
| if (!user) return done(null, false, { message: 'Incorrect username' }); | ||
|
|
||
| // Compare passwords | ||
| const isMatch = await bcrypt.compare(password, user.password); | ||
| if (!isMatch) return done(null, false, { message: 'Incorrect password' }); | ||
|
|
||
| return done(null, user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Login will fail if clients send email; set usernameField: 'email' and query by email.
Elsewhere (routes/tests) the login uses email. Update the strategy to read req.body.email and prefer the model’s comparePassword if available to keep hashing logic centralized.
-passport.use(
- new LocalStrategy(async (username, password, done) => {
+passport.use(
+ new LocalStrategy(
+ { usernameField: 'email', passwordField: 'password', session: true },
+ async (email, password, done) => {
try {
- // Find user by username
- const user = await User.findOne({ username });
- if (!user) return done(null, false, { message: 'Incorrect username' });
+ // Find user by email
+ const user = await User.findOne({ email });
+ if (!user) return done(null, false, { message: 'Email not found' });
- // Compare passwords
- const isMatch = await bcrypt.compare(password, user.password);
- if (!isMatch) return done(null, false, { message: 'Incorrect password' });
+ // Prefer model method if defined
+ const isMatch = typeof user.comparePassword === 'function'
+ ? await user.comparePassword(password)
+ : await bcrypt.compare(password, user.password);
+ if (!isMatch) return done(null, false, { message: 'Incorrect password' });
- return done(null, user);
- } catch (err) {
- return done(err);
- }
- })
+ return done(null, user);
+ } catch (err) {
+ return done(err);
+ }
+ }
+ )
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| passport.use( | |
| new LocalStrategy(async (username, password, done) => { | |
| try { | |
| // Find user by username | |
| const user = await User.findOne({ username }); | |
| if (!user) return done(null, false, { message: 'Incorrect username' }); | |
| // Compare passwords | |
| const isMatch = await bcrypt.compare(password, user.password); | |
| if (!isMatch) return done(null, false, { message: 'Incorrect password' }); | |
| return done(null, user); | |
| passport.use( | |
| new LocalStrategy( | |
| { usernameField: 'email', passwordField: 'password', session: true }, | |
| async (email, password, done) => { | |
| try { | |
| // Find user by email | |
| const user = await User.findOne({ email }); | |
| if (!user) return done(null, false, { message: 'Email not found' }); | |
| // Prefer model method if defined | |
| const isMatch = typeof user.comparePassword === 'function' | |
| ? await user.comparePassword(password) | |
| : await bcrypt.compare(password, user.password); | |
| if (!isMatch) return done(null, false, { message: 'Incorrect password' }); | |
| return done(null, user); | |
| } catch (err) { | |
| return done(err); | |
| } | |
| } | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In backend/config/passport.js lines 6-17, the LocalStrategy currently expects a
username and uses bcrypt.compare; change the strategy to use usernameField:
'email' (and passwordField if needed) so it reads the client's email, query User
by { email } instead of { username }, and replace direct bcrypt.compare with the
model's comparePassword method when present (e.g., await
user.comparePassword(password)) to centralize hashing logic and keep behavior
consistent with routes/tests.
| <a href="https://vite.dev" target="_blank"> | ||
| <img src={viteLogo} className="logo" alt="Vite logo" /> | ||
| </a> | ||
| <a href="https://react.dev" target="_blank"> | ||
| <img src={reactLogo} className="logo react" alt="React logo" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add rel="noopener noreferrer" to external links
Prevents window.opener attacks.
Apply:
- <a href="https://vite.dev" target="_blank">
+ <a href="https://vite.dev" target="_blank" rel="noopener noreferrer">
<img src={viteLogo} className="logo" alt="Vite logo" />
</a>
- <a href="https://react.dev" target="_blank">
+ <a href="https://react.dev" target="_blank" rel="noopener noreferrer">
<img src={reactLogo} className="logo react" alt="React logo" />
</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://vite.dev" target="_blank"> | |
| <img src={viteLogo} className="logo" alt="Vite logo" /> | |
| </a> | |
| <a href="https://react.dev" target="_blank"> | |
| <img src={reactLogo} className="logo react" alt="React logo" /> | |
| <a href="https://vite.dev" target="_blank" rel="noopener noreferrer"> | |
| <img src={viteLogo} className="logo" alt="Vite logo" /> | |
| </a> | |
| <a href="https://react.dev" target="_blank" rel="noopener noreferrer"> | |
| <img src={reactLogo} className="logo react" alt="React logo" /> | |
| </a> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 12-12: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
[error] 15-15: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In client/src/App.jsx around lines 12 to 16, the external anchor tags opening in
a new tab lack rel="noopener noreferrer"; update each <a ... target="_blank"> to
include rel="noopener noreferrer" to prevent window.opener attacks and ensure
safe external linking.
| const user = await User.findOne({ username }); | ||
| if (!user) return done(null, false, { message: 'Incorrect username.' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field mismatch with other authentication components.
This configuration uses username for authentication, but other files suggest the system also supports email-based login. The backend/config/passportConfig.js uses email, creating inconsistency.
Ensure consistent field usage across authentication components or support multiple strategies if both username and email login are required.
🤖 Prompt for AI Agents
In config/passport.js around lines 10-11, the authentication lookup only checks
User.findOne({ username }) causing inconsistency with
backend/config/passportConfig.js which uses email; update the lookup to support
the same field(s) across the app by either replacing the username lookup with
email (User.findOne({ email })) if the system standardizes on email, or change
it to accept both by deriving a single credential variable (e.g., credential)
and querying User.findOne({ $or: [{ username: credential }, { email: credential
}] }); also adjust the "Incorrect username." message to a neutral message like
"Incorrect credentials." to cover both cases.
| // Logout | ||
| router.get('/logout', (req, res) => { | ||
| req.logout(err => { | ||
| if (err) return res.status(500).json({ error: err.message }); | ||
| res.json({ msg: 'Logged out successfully' }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use POST for logout and destroy session/cookie
GET logout is CSRF-prone. Destroy the session and clear the cookie.
Apply:
-router.get('/logout', (req, res) => {
+router.post('/logout', (req, res) => {
req.logout(err => {
- if (err) return res.status(500).json({ error: err.message });
- res.json({ msg: 'Logged out successfully' });
+ if (err) return res.status(500).json({ message: err.message });
+ req.session?.destroy(() => {
+ res.clearCookie('connect.sid');
+ res.json({ message: 'Logged out successfully' });
+ });
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Logout | |
| router.get('/logout', (req, res) => { | |
| req.logout(err => { | |
| if (err) return res.status(500).json({ error: err.message }); | |
| res.json({ msg: 'Logged out successfully' }); | |
| }); | |
| }); | |
| // Logout | |
| router.post('/logout', (req, res) => { | |
| req.logout(err => { | |
| if (err) return res.status(500).json({ message: err.message }); | |
| req.session?.destroy(() => { | |
| res.clearCookie('connect.sid'); | |
| res.json({ message: 'Logged out successfully' }); | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In src/Routes/auth.js around lines 41 to 47, the logout route is currently a GET
and should be changed to a POST that destroys the session and clears the cookie:
update router.get('/logout') to router.post('/logout'), call req.logout (if
using passport) and then req.session.destroy with error handling, clear the
session cookie via res.clearCookie using your session cookie name, and return a
200 JSON response on success or a 500 JSON error on failure; ensure errors from
req.logout or session destruction are handled and sent in the response.
| import { useState } from 'react'; | ||
| import axios from 'axios'; | ||
| import { useNavigate } from 'react-router-dom'; | ||
|
|
||
| export default function Signup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid hardcoded URLs; use an env-backed base URL.
Makes local/dev/prod switching trivial and aligns with CORS/credentials.
Apply:
import { useNavigate } from 'react-router-dom';
+const API_URL = import.meta.env.VITE_API_URL || 'http://localhost:5000';
export default function Signup() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { useState } from 'react'; | |
| import axios from 'axios'; | |
| import { useNavigate } from 'react-router-dom'; | |
| export default function Signup() { | |
| import { useState } from 'react'; | |
| import axios from 'axios'; | |
| import { useNavigate } from 'react-router-dom'; | |
| const API_URL = import.meta.env.VITE_API_URL || 'http://localhost:5000'; | |
| export default function Signup() { |
🤖 Prompt for AI Agents
In src/Signup.jsx around lines 1 to 5, the code currently relies on hardcoded
URLs for API calls; replace that by reading a base URL from an environment
variable (e.g. process.env.REACT_APP_API_BASE_URL or VITE_API_BASE_URL depending
on the build system) and use it when constructing request URLs. Create an axios
instance with baseURL set to that env value (and fallback to '' for same-origin)
or prepend the env var to endpoint paths, then update all axios calls in this
component to use the instance or the env-backed URLs; ensure environment
variable name is documented and that no plaintext URLs remain in the file.
| const [username, setUsername] = useState(''); | ||
| const [password, setPassword] = useState(''); | ||
| const navigate = useNavigate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Collect email on the form to satisfy backend schema.
Apply:
- const [username, setUsername] = useState('');
- const [password, setPassword] = useState('');
+ const [username, setUsername] = useState('');
+ const [email, setEmail] = useState('');
+ const [password, setPassword] = useState('');- <input type="text" placeholder="Username" value={username} onChange={(e) => setUsername(e.target.value)} required />
- <input type="password" placeholder="Password" value={password} onChange={(e) => setPassword(e.target.value)} required />
+ <input type="text" placeholder="Username" value={username} onChange={(e) => setUsername(e.target.value)} required />
+ <input type="email" placeholder="Email" value={email} onChange={(e) => setEmail(e.target.value)} required />
+ <input type="password" placeholder="Password" value={password} onChange={(e) => setPassword(e.target.value)} required />Also applies to: 24-25
🤖 Prompt for AI Agents
In src/Signup.jsx around lines 6-8 (and also update lines 24-25), the component
only tracks username and password but the backend schema requires an email; add
a new useState hook for email (e.g., const [email, setEmail] = useState('')),
wire setEmail to the email input element in the JSX form (insert an email input
alongside username/password at lines ~24-25), and update the form submit handler
to include email in the payload sent to the backend (and any validation or
required attribute on the input as appropriate).
| const handleSignup = async (e) => { | ||
| e.preventDefault(); | ||
| try { | ||
| const res = await axios.post('http://localhost:5000/auth/signup', { username, password }, { withCredentials: true }); | ||
| alert(res.data.msg); | ||
| navigate('/login'); | ||
| } catch (err) { | ||
| alert(err.response?.data?.msg || 'Signup failed'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix API contract: wrong path, missing email, and incorrect response key.
Backend expects { username, email, password } at POST /api/auth/signup and returns { message }. Your code posts to /auth/signup with only username/password and reads res.data.msg.
Apply:
- const res = await axios.post('http://localhost:5000/auth/signup', { username, password }, { withCredentials: true });
- alert(res.data.msg);
+ const res = await axios.post(`${API_URL}/api/auth/signup`, { username, email, password }, { withCredentials: true });
+ alert(res.data.message);Also change error handling:
- alert(err.response?.data?.msg || 'Signup failed');
+ alert(err.response?.data?.message || 'Signup failed');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Signup.jsx around lines 10 to 19, the signup request uses the wrong
endpoint, omits the email field and reads the wrong response key; change the
POST URL to '/api/auth/signup', include email in the request body alongside
username and password, and read res.data.message (not res.data.msg) after a
successful response; also update the catch to use err.response?.data?.message ||
'Signup failed' for error alerts while keeping withCredentials and the
navigate('/login') flow.
|
Too many changes in one commit. Split commits for small changes... https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines |
This pull request adds the backend setup for authentication in the GitHub Tracker project.
Changes include:
server.jsto initialize the Node.js server.backend/config/db.jsbackend/config/passport.jsbackend/config/passportConfig.js(updated)backend/routes/auth.js.github-tracker-authfolder with initial setup.These changes set up the backend authentication system and make the project ready for further integration and testing.
Please review and merge.
Summary by CodeRabbit
New Features
Chores